Skip to content

[Bugfix] HCI response to disconnect of unknown ID should return success.#414

Open
h2zero wants to merge 1 commit intomasterfrom
bugfix/disconnect-unk-conn-id
Open

[Bugfix] HCI response to disconnect of unknown ID should return success.#414
h2zero wants to merge 1 commit intomasterfrom
bugfix/disconnect-unk-conn-id

Conversation

@h2zero
Copy link
Copy Markdown
Owner

@h2zero h2zero commented Mar 27, 2026

  • Update macro use to use conversion macros.

Summary by CodeRabbit

  • Bug Fixes
    • Improved detection and handling of missing PIN/key errors during secure connections to avoid false failures and unintended retries.
    • Treated an additional unknown-connection-ID termination response as non-fatal to prevent unnecessary error reports during disconnects.
    • Enhanced connection and disconnection logging with clearer, human-readable messages to aid troubleshooting.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 27, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: dccc4519-4a98-495d-9777-a706ca982dbc

📥 Commits

Reviewing files that changed from the base of the PR and between 0a0dbaa and 91251ea.

📒 Files selected for processing (2)
  • src/NimBLEClient.cpp
  • src/NimBLEServer.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/NimBLEClient.cpp

📝 Walkthrough

Walkthrough

Avast: standardized HCI error comparisons to BLE_HS_HCI_ERR(...), enhanced client secureConnection failure logs with human-readable strings, and adjusted client/server disconnect handling to treat BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) as non-fatal in disconnect flows. Yarrr, concise and factual (≤50 words).

Changes

Cohort / File(s) Summary
Client HCI & Logging
src/NimBLEClient.cpp
Replaced base+offset HCI error comparisons with BLE_HS_HCI_ERR(...) (PIN-key-missing, conn-establishment); added human-readable return string to secureConnection() failure logs.
Client GAP / Encryption Handling
src/NimBLEClient.cpp
Adjusted handleGapEvent(...) disconnect detection and BLE_GAP_EVENT_ENC_CHANGE branches to use BLE_HS_HCI_ERR(...) checks; updated retry/delete-peer logic comparisons accordingly.
Server Disconnect Handling
src/NimBLEServer.cpp
Refactored ble_gap_terminate error handling to treat BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) as non-fatal; other rc values log error and return failure.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

Yarrr, the HCI codes be tamed and bright,
Logs now sing clear by lantern light,
Unknown IDs no longer cause a fright,
Clients and servers steady the night,
Code patched, the ship sails right.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main bugfix: treating HCI disconnect responses for unknown connection IDs as successful rather than errors, which is the core change across both modified files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch bugfix/disconnect-unk-conn-id

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/NimBLEServer.cpp (1)

336-343: ⚠️ Potential issue | 🔴 Critical

Arrr, matey! The server be needin' cleanup when the controller says it knows not the connection.

Line 338 treats BLE_ERR_UNK_CONN_ID as success, but this be leavin' m_connectedPeers stale forever. When the controller reports it knows not the connection, there'll be no BLE_GAP_EVENT_DISCONNECT event comin' back to clean up yer bookkeeping. This means getConnectedCount(), getPeerDevices(), advertiseOnDisconnect, and the optional m_pClient->m_connHandle mirror stay stale. The client handles this right by settin' m_connStatus = DISCONNECTED on the spot (line 387); the server should do the same for m_connectedPeers before returnin' true.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/NimBLEServer.cpp` around lines 336 - 343, NimBLEServer::disconnect
currently treats BLE_ERR_UNK_CONN_ID as a non-error and returns true without
cleaning up state; update disconnect(uint16_t connHandle, uint8_t reason) so
when ble_gap_terminate returns BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) you remove
the peer from m_connectedPeers (and perform the same per-peer cleanup you would
on BLE_GAP_EVENT_DISCONNECT) and update any mirror state (e.g., clear the peer's
m_connHandle, set its connection status to DISCONNECTED) before returning true
so getConnectedCount(), getPeerDevices(), advertiseOnDisconnect and related
logic reflect the disconnection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/NimBLEClient.cpp`:
- Around line 381-387: When ble_gap_terminate(...) returns BLE_ERR_UNK_CONN_ID
the code sets m_connStatus to DISCONNECTED without firing the
BLE_GAP_EVENT_DISCONNECT cleanup path, which can orphan clients when
deleteOnDisconnect is true; modify the post-terminate branch in NimBLEClient.cpp
(where BLE_ERR_UNK_CONN_ID is checked) to perform the same cleanup the GAP
disconnect handler does: detect the BLE_ERR_UNK_CONN_ID fast path, check
this->deleteOnDisconnect (and any relevant client flags), and invoke the same
cleanup sequence used by the GAP handler (or call the same helper used by that
handler) so delete-on-disconnect clients are freed immediately rather than
relying on an event. Ensure m_lastErr and m_connStatus are still set
appropriately and reuse existing cleanup logic to avoid duplication.

---

Outside diff comments:
In `@src/NimBLEServer.cpp`:
- Around line 336-343: NimBLEServer::disconnect currently treats
BLE_ERR_UNK_CONN_ID as a non-error and returns true without cleaning up state;
update disconnect(uint16_t connHandle, uint8_t reason) so when ble_gap_terminate
returns BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) you remove the peer from
m_connectedPeers (and perform the same per-peer cleanup you would on
BLE_GAP_EVENT_DISCONNECT) and update any mirror state (e.g., clear the peer's
m_connHandle, set its connection status to DISCONNECTED) before returning true
so getConnectedCount(), getPeerDevices(), advertiseOnDisconnect and related
logic reflect the disconnection.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f943e235-e0ba-400f-b17a-09b83939accb

📥 Commits

Reviewing files that changed from the base of the PR and between afe1221 and 4e27a4f.

📒 Files selected for processing (2)
  • src/NimBLEClient.cpp
  • src/NimBLEServer.cpp

Comment on lines +381 to +387
if (rc != 0 && rc != BLE_HS_ENOTCONN && rc != BLE_HS_EALREADY && rc != BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID)) {
NIMBLE_LOGE(LOG_TAG, "ble_gap_terminate failed: rc=%d %s", rc, NimBLEUtils::returnCodeToString(rc));
m_lastErr = rc;
return false;
}

m_connStatus = (rc == BLE_HS_ENOTCONN) ? DISCONNECTED : DISCONNECTING;
m_connStatus = (rc == BLE_HS_ENOTCONN || rc == BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID)) ? DISCONNECTED : DISCONNECTING;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== NimBLEClient::disconnect ==\n'
sed -n '375,389p' src/NimBLEClient.cpp

printf '\n== NimBLEDevice::deleteClient ==\n'
sed -n '352,400p' src/NimBLEDevice.cpp

printf '\n== deleteOnDisconnect usages ==\n'
rg -n -C2 'deleteOnDisconnect|deleteClient\(' src/NimBLEClient.cpp src/NimBLEDevice.cpp

Repository: h2zero/esp-nimble-cpp

Length of output: 5154


🏁 Script executed:

rg -n 'deleteOnDisconnect|BLE_GAP_EVENT_DISCONNECT|BLE_ERR_UNK_CONN_ID' src/NimBLEClient.cpp src/NimBLEDevice.cpp -A 3 -B 1

Repository: h2zero/esp-nimble-cpp

Length of output: 3169


🏁 Script executed:

# Check if there's cleanup that happens outside GAP events
rg -n 'BLE_GAP_EVENT_DISCONNECT' src/ -A 30 | head -100

Repository: h2zero/esp-nimble-cpp

Length of output: 6851


🏁 Script executed:

# Look for any task or timer-based cleanup
rg -n 'deinit|cleanup|delete.*client|purge' src/NimBLEDevice.cpp -i | head -20

Repository: h2zero/esp-nimble-cpp

Length of output: 681


🏁 Script executed:

# Check if there are any comments about BLE_ERR_UNK_CONN_ID behavior
rg -n 'BLE_ERR_UNK_CONN_ID' src/ -B 3 -A 3

Repository: h2zero/esp-nimble-cpp

Length of output: 2008


🏁 Script executed:

# Check what deleteOnDisconnect does when client is already DISCONNECTED
rg -n 'DISCONNECTED.*deleteOnDisconnect|deleteOnDisconnect.*DISCONNECTED' src/ -i

Repository: h2zero/esp-nimble-cpp

Length of output: 276


🏁 Script executed:

# Search for any cleanup path that handles deleteOnDisconnect outside GAP event
rg -n 'deleteOnDisconnect' src/ -B 2 -A 5

Repository: h2zero/esp-nimble-cpp

Length of output: 4568


Arrr, the BLE_ERR_UNK_CONN_ID fast path creates an orphaned client, matey!

Aye, when ble_gap_terminate() returns BLE_ERR_UNK_CONN_ID (line 387), the client transitions straight to DISCONNECTED without ever firing a BLE_GAP_EVENT_DISCONNECT event. This be a problem: if NimBLEDevice::deleteClient() sets deleteOnDisconnect=true and calls disconnect(), the flag be set but the GAP handler that checks it (line 1085 in NimBLEClient.cpp) will never run. The client be left orphaned with cleanup forever deferred.

Either add explicit cleanup at line 387 when BLE_ERR_UNK_CONN_ID be detected (check deleteOnDisconnect and clean up like the GAP handler does), or ensure NimBLEDevice::deleteClient() handles the DISCONNECTED state directly without relying on the event path.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/NimBLEClient.cpp` around lines 381 - 387, When ble_gap_terminate(...)
returns BLE_ERR_UNK_CONN_ID the code sets m_connStatus to DISCONNECTED without
firing the BLE_GAP_EVENT_DISCONNECT cleanup path, which can orphan clients when
deleteOnDisconnect is true; modify the post-terminate branch in NimBLEClient.cpp
(where BLE_ERR_UNK_CONN_ID is checked) to perform the same cleanup the GAP
disconnect handler does: detect the BLE_ERR_UNK_CONN_ID fast path, check
this->deleteOnDisconnect (and any relevant client flags), and invoke the same
cleanup sequence used by the GAP handler (or call the same helper used by that
handler) so delete-on-disconnect clients are freed immediately rather than
relying on an event. Ensure m_lastErr and m_connStatus are still set
appropriately and reuse existing cleanup logic to avoid duplication.

@h2zero h2zero force-pushed the bugfix/disconnect-unk-conn-id branch from 4e27a4f to 69cff9a Compare March 28, 2026 00:15
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
src/NimBLEClient.cpp (1)

381-387: ⚠️ Potential issue | 🟠 Major

Arrr, BLE_ERR_UNK_CONN_ID still leaves the client stuck in DISCONNECTING.

Line 381 now treats BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) as success, but Line 387 still maps it to DISCONNECTING. connect() only accepts DISCONNECTED at Lines 261-264, so this path now depends on a later GAP disconnect event to restore state. If that event never arrives for an unknown handle, the client stays wedged and reconnects get rejected. I’d treat BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) the same as BLE_HS_ENOTCONN in the local state machine.

🏴‍☠️ Minimal state fix
-    m_connStatus = rc == BLE_HS_ENOTCONN ? DISCONNECTED : DISCONNECTING;
+    m_connStatus = (rc == BLE_HS_ENOTCONN || rc == BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID))
+                       ? DISCONNECTED
+                       : DISCONNECTING;
#!/bin/bash
set -euo pipefail

client_file="$(fd -p 'NimBLEClient\.cpp$' src | head -n1)"
device_file="$(fd -p 'NimBLEDevice\.cpp$' src | head -n1)"

printf '\n== disconnect() success path ==\n'
sed -n '379,388p' "$client_file"

printf '\n== connect() precondition ==\n'
sed -n '255,265p' "$client_file"

printf '\n== all m_connStatus transitions in NimBLEClient.cpp ==\n'
rg -n -C2 'm_connStatus\s*=' "$client_file"

printf '\n== deleteClient / deleteOnDisconnect handling ==\n'
rg -n -C3 'deleteClient\s*\(|deleteOnDisconnect|deleteOnConnectFail' "$device_file" "$client_file"

Expected result: ye should see the unknown-connection success path landing in DISCONNECTING, connect() only allowing DISCONNECTED, and whether any separate delete-on-disconnect cleanup exists outside this branch.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/NimBLEClient.cpp` around lines 381 - 387, The code treats
BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) as a non-error return from
ble_gap_terminate() but still sets m_connStatus to DISCONNECTING, which can
leave the client wedged; update the disconnect() success-path in
NimBLEClient.cpp so that when rc equals BLE_HS_ENOTCONN OR
BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) you set m_connStatus = DISCONNECTED (same as
the ENOTCONN case) and preserve m_lastErr handling, so connect() (which checks
for DISCONNECTED) will allow reconnection immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/NimBLEClient.cpp`:
- Around line 381-387: The code treats BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) as a
non-error return from ble_gap_terminate() but still sets m_connStatus to
DISCONNECTING, which can leave the client wedged; update the disconnect()
success-path in NimBLEClient.cpp so that when rc equals BLE_HS_ENOTCONN OR
BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) you set m_connStatus = DISCONNECTED (same as
the ENOTCONN case) and preserve m_lastErr handling, so connect() (which checks
for DISCONNECTED) will allow reconnection immediately.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bd099957-ae8e-4cd2-891c-8f6cd7eaf01c

📥 Commits

Reviewing files that changed from the base of the PR and between 4e27a4f and 69cff9a.

📒 Files selected for processing (2)
  • src/NimBLEClient.cpp
  • src/NimBLEServer.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/NimBLEServer.cpp

@h2zero h2zero force-pushed the bugfix/disconnect-unk-conn-id branch 2 times, most recently from 5f8a1be to 0a0dbaa Compare March 28, 2026 16:45
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
src/NimBLEClient.cpp (1)

381-393: ⚠️ Potential issue | 🟠 Major

Arrr, this “success” branch still skips local disconnect finalization.

For BLE_HS_ENOTCONN and BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID), this returns true without clearing m_connHandle, setting m_connStatus = DISCONNECTED, or reaching the deleteOnDisconnect path that only runs in the BLE_GAP_EVENT_DISCONNECT handler on Lines 1036-1099. If no disconnect event follows those codes, a later connect() can still fail the guard on Lines 261-264 and self-delete never fires. Please route these cases through the same disconnect-finalization path instead of a bare success return.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/NimBLEClient.cpp` around lines 381 - 393, The switch that treats
BLE_HS_ENOTCONN and BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) as a simple success must
instead perform the same local disconnect finalization as the
BLE_GAP_EVENT_DISCONNECT path: clear m_connHandle, set m_connStatus =
DISCONNECTED, and trigger the deleteOnDisconnect behavior (or call the same
helper used in the BLE_GAP_EVENT_DISCONNECT handler) before returning true;
update the case arms in the switch in NimBLEClient.cpp so they do not just
return true but run that finalization logic to avoid leaving the client in a
half-disconnected state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/NimBLEClient.cpp`:
- Around line 381-393: The switch that treats BLE_HS_ENOTCONN and
BLE_HS_HCI_ERR(BLE_ERR_UNK_CONN_ID) as a simple success must instead perform the
same local disconnect finalization as the BLE_GAP_EVENT_DISCONNECT path: clear
m_connHandle, set m_connStatus = DISCONNECTED, and trigger the
deleteOnDisconnect behavior (or call the same helper used in the
BLE_GAP_EVENT_DISCONNECT handler) before returning true; update the case arms in
the switch in NimBLEClient.cpp so they do not just return true but run that
finalization logic to avoid leaving the client in a half-disconnected state.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 19de5ead-c538-42bf-89b0-86310d5d6146

📥 Commits

Reviewing files that changed from the base of the PR and between 5f8a1be and 0a0dbaa.

📒 Files selected for processing (2)
  • src/NimBLEClient.cpp
  • src/NimBLEServer.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/NimBLEServer.cpp

@h2zero h2zero force-pushed the bugfix/disconnect-unk-conn-id branch from 0a0dbaa to 91251ea Compare March 28, 2026 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant